Add EValue::tryTo<T>() for all EValue payload types#19036
Conversation
|
Stack from ghstack (oldest at bottom): |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19036
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 133 PendingAs of commit 6e4eab1 with merge base 2d53535 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR adds non-aborting, Result-returning “tryTo*” accessors to executorch::runtime::EValue so callers can safely handle tag mismatches (and certain invalid payload states) without terminating the process—especially important when processing untrusted .pte inputs.
Changes:
- Added
Result-returningtryTo*accessors for EValue payload types, plus a templatedtryTo<T>()dispatcher. - Generalized
tryToOptional<T>()beyond Tensor to work with any supported payload type viatryTo<T>(). - Added unit tests covering success/mismatch paths for several new accessors and the widened
tryToOptional<T>().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| runtime/core/evalue.h | Introduces tryTo* APIs, tryTo<T>() specializations, and generic tryToOptional<T>(). |
| runtime/core/test/evalue_test.cpp | Adds test coverage for new tryTo* APIs and the generalized tryToOptional<T>(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // List tryTo* — mismatch paths. Success paths require building a | ||
| // BoxedEvalueList or ArrayRef host object, which the non-list tests above | ||
| // already cover via the shared tag-check logic; one mismatch test per list | ||
| // type is enough to exercise the added code. | ||
|
|
||
| TEST_F(EValueTest, TryToIntListTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToIntList(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToDoubleListTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToDoubleList(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToBoolListTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToBoolList(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToTensorListTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToTensorList(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToListOptionalTensorTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToListOptionalTensor(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } | ||
|
|
||
| TEST_F(EValueTest, TryToStringTypeMismatch) { | ||
| EValue e(static_cast<int64_t>(42)); | ||
| auto result = e.tryToString(); | ||
| EXPECT_EQ(result.error(), executorch::runtime::Error::InvalidType); | ||
| } |
There was a problem hiding this comment.
The new tryToString()/tryTo*List() accessors add an Error::InvalidState path for null internal payload pointers, but the added tests only cover tag-mismatch for string/lists (and no success case for tryToString). Consider adding explicit tests that (1) a well-formed String/List returns ok(), and (2) a manually-constructed EValue with the right tag but a null payload pointer returns Error::InvalidState (rather than aborting).
| Result<executorch::aten::Tensor> tryToTensor() const { | ||
| if (!isTensor()) { | ||
| return Error::InvalidType; | ||
| } | ||
| return payload.as_tensor; | ||
| } | ||
|
|
There was a problem hiding this comment.
tryToTensor() returns a Tensor by value from a const EValue, which will typically incur a Tensor copy/refcount bump even when the caller only needs to inspect the tensor. If the goal is a safe, non-aborting accessor for performance-sensitive paths, consider returning a pointer type (e.g., Result<const Tensor*>) or adding an rvalue overload (tryToTensor() &&) that can move out, to avoid extra copies where possible.
| Result<executorch::aten::Tensor> tryToTensor() const { | |
| if (!isTensor()) { | |
| return Error::InvalidType; | |
| } | |
| return payload.as_tensor; | |
| } | |
| Result<const executorch::aten::Tensor*> tryToTensor() const& { | |
| if (!isTensor()) { | |
| return Error::InvalidType; | |
| } | |
| return &payload.as_tensor; | |
| } | |
| Result<executorch::aten::Tensor> tryToTensor() && { | |
| if (!isTensor()) { | |
| return Error::InvalidType; | |
| } | |
| auto res = std::move(payload.as_tensor); | |
| clearToNone(); | |
| return res; | |
| } |
| /** | ||
| * Result-returning equivalent of `toOptional<T>()`. None maps to an empty | ||
| * optional; any other tag that doesn't match T propagates `tryTo<T>()`'s | ||
| * error (`Error::InvalidType`). | ||
| */ | ||
| template <typename T> | ||
| inline Result<std::optional<T>> tryToOptional() const { | ||
| if (this->isNone()) { | ||
| return std::optional<T>(executorch::aten::nullopt); | ||
| } | ||
| auto r = this->tryTo<T>(); | ||
| if (!r.ok()) { | ||
| return r.error(); | ||
| } | ||
| return std::optional<T>(std::move(r.get())); |
There was a problem hiding this comment.
The docstring for tryToOptional() says it propagates tryTo()'s error (Error::InvalidType), but tryTo() can also return other errors (e.g., tryToString()/tryTo*List() return Error::InvalidState on null payload pointers). Consider updating the comment to avoid implying InvalidType is the only possible error.
| tryToTensorList) | ||
| EVALUE_DEFINE_TRY_TO( | ||
| executorch::aten::ArrayRef<std::optional<executorch::aten::Tensor>>, | ||
| tryToListOptionalTensor) |
There was a problem hiding this comment.
tryTo() is described as mirroring to(), but the tryTo dispatcher only specializes the non-optional payload types. to() also supports std::optional<...> specializations (e.g., std::optional, std::optional<ArrayRef<int64_t>>, etc.), so tryTo<std::optional<...>>() will currently be an undefined/unspecialized template and fail at link-time. Consider adding EVALUE_DEFINE_TRY_TO specializations for the same optional-returning types, or explicitly deleting/guarding unsupported instantiations to produce a clear compile-time error and steer users to tryToOptional().
| tryToListOptionalTensor) | |
| tryToListOptionalTensor) | |
| #define EVALUE_DEFINE_TRY_TO_OPTIONAL(cpp_type) \ | |
| template <> \ | |
| inline Result<std::optional<cpp_type>> EValue::tryTo< \ | |
| std::optional<cpp_type>>() const { \ | |
| return tryToOptional<cpp_type>(); \ | |
| } | |
| EVALUE_DEFINE_TRY_TO_OPTIONAL(executorch::aten::Tensor) | |
| EVALUE_DEFINE_TRY_TO_OPTIONAL(executorch::aten::ArrayRef<int64_t>) | |
| EVALUE_DEFINE_TRY_TO_OPTIONAL(executorch::aten::ArrayRef<double>) | |
| EVALUE_DEFINE_TRY_TO_OPTIONAL(executorch::aten::ArrayRef<bool>) | |
| EVALUE_DEFINE_TRY_TO_OPTIONAL( | |
| executorch::aten::ArrayRef<executorch::aten::Tensor>) | |
| EVALUE_DEFINE_TRY_TO_OPTIONAL( | |
| executorch::aten::ArrayRef<std::optional<executorch::aten::Tensor>>) | |
| #undef EVALUE_DEFINE_TRY_TO_OPTIONAL |
Introduce
tryToversions of each Evalue accessor.Current accessors
toTensor(), etc. abort withET_CHECK_MSGwhen it's an incorrect type.API additions:
tryToTensor (already present, kept), tryToIntList, tryToBoolList,
tryToDoubleList, tryToTensorList, tryToListOptionalTensor,
tryToScalarType, tryToMemoryFormat, tryToLayout, tryToDevice.
Tag mismatch returns Error::InvalidType; null list/string payload
returns Error::InvalidState.
EVALUE_DEFINE_TRY_TO macro kept adjacent to EVALUE_DEFINE_TO so drift
between the two surfaces is visible at review time.
to tryTo() so it works for any supported payload type.
Tests cover success + mismatch paths for each new accessor, plus the
widened tryToOptional() path.
Authored-with: Claude